Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize WaitForTransaction #132

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ice-coldbell
Copy link

@ice-coldbell ice-coldbell commented Feb 23, 2025

Description

  • Optimize WaitForTransaction.

Adopt the WaitForTransaction polling method implemented via wait_by_hash api in the Typescript SDK.

However, there is a concern that this might conflict with the PR using Golang context for HTTP requests.
In that case, a ctx should be accepted and used for polling.

Test Plan

  • Verify that WaitTransactionByHash operates correctly.
  • Confirm that PollForTransaction produces the same results as before.

Related Links

@ice-coldbell ice-coldbell requested review from gregnazario and a team as code owners February 23, 2025 17:15
@ice-coldbell
Copy link
Author

Question

I'm not entirely sure how the server-side behavior of wait_by_hash works.
Therefore, I'm wondering whether the PollForTransaction implementation should call TransactionByHash first and then WaitTransactionByHash, or if it's acceptable to skip TransactionByHash and directly call the WaitTransactionByHash API.

@gregnazario
Copy link
Contributor

Question

I'm not entirely sure how the server-side behavior of wait_by_hash works. Therefore, I'm wondering whether the PollForTransaction implementation should call TransactionByHash first and then WaitTransactionByHash, or if it's acceptable to skip TransactionByHash and directly call the WaitTransactionByHash API.

The TS SDK has this implementation.

You'll want to call "wait_by_hash" first, and follow it up with the shorter calls.

The Tl;dr is that wait by hash will keep a connection open for that time, and really is not necessary after the first one. It's better from a long term to call the TransactionByHash call.

@ice-coldbell
Copy link
Author

Fixed the test-related issues.
Since it's currently not possible to distinguish between an API Error and an HTTP Error, I implemented it in the same way as our existing error handling method.

Comparing error message strings is not a recommended approach for error handling, so I believe that once an API Error Type is implemented in the future, can add additional handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants